Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correção de bug em calculo de fretes #184

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nichmorgan
Copy link

O cálculo do frete (calculate_freights) estava enviando o type de pacote da classe e não o tipo de pacote de envio.

O cálculo do frete (calculate_freights) estava enviando o type de pacote da classe e não o tipo de pacote de envio.
@gfabrizio
Copy link
Contributor

Olá, obrigado pela contribuição. Por favor, você pode acrescentar teste dessa modificação? em tempo, me aprece que com essa modificação alguns testes como esse

def test_calculate_freights(client, posting_card, package):
deveriam quebrar, pode verificar esses testes?

gilsonbp
gilsonbp previously approved these changes Apr 2, 2020
@gilsonbp gilsonbp dismissed their stale review April 2, 2020 13:22

Aguardando os tests

@nichmorgan
Copy link
Author

Olá, realizei os testes, os únicos quebrados não tem relação alguma com a classe de testes client. Como poderia exportar os testes para vocês? É o meu primeiro pull request.

@gfabrizio
Copy link
Contributor

Olá @nichmorgan, tudo bem?
você pode escrever os testes da modificação feita no seu pr em algum dos nossos arquivos de teste, e.g: https://github.com/olist/correios/blob/419e24b658ed6cda36ee2928b601f222e4df0218/tests/test_client.py também pode executar os testes com o comando make test.
Para o pr ser aprovado e feito seu merge você deve escrever testes específicos para a sua mod e 100% dos testes devem passar.

@memachado
Copy link

Corrijam-me se eu estiver errado, principalmente por ser novato em python,

Mas essa PR passa em todos os testes já existentes (acabei de rodar), então ela deveria ser aprovada, nao?

Além disso, a modificação sugeriada é para corrigir um possível bug, que inclusive está acontecendo comigo.

@nichmorgan Achou outra saida para o erro que encontrou, ou apenas aplicou a modificação localmente? Ou ainda nem era erro, e achou o meio certo de utilizar?

Abraços.

@nichmorgan
Copy link
Author

Exatamente @parannoide, eu mesmo estou usando meu fork, pq não sei mais como aprovar aqui, talvez inexperiência minha.

@erikhenrique
Copy link

@nichmorgan e @parannoide gostaria de dar meus 2 cents, mesmo não sendo o mantenedor do repositório.
Antes de tudo, sua contribuição é realmente muito boa para a comunidade open source, obrigado!

Pelo que checkei na doc dos correios, na página 4/16 essa parte do código realmente está errada e faz sentido a alteração.

Sobre os testes.
Uma coisa que precisamos ter claro é que nem sempre que recebemos sucesso da suit de testes quando alteramos algo, quer dizer que a nossa alteração está correta. Algo esperado em qualquer repositório de código que tenha testes é que quando alteramos uma parte do código, o esperado é que os testes quebrem. Se isso não acontece, algo está errado com a nossa cobertura de testes.

Pelo que pude olhar aqui, encontrei alguns problemas:

  • Não existe nenhum teste unitário para validar exatamente essa chamada aos correios com o parâmetro correto de package.
  • O teste para a chamada dos correios está utilizando vcr, isso faz com que tanto a chamada e resposta com os correios sejam mockadas. Mitigando um pouco o erro.
    Então uma chamada aos correios passando o parâmetro de tipo de pacote como 1 ou 2 não está fazendo diferença para os asserts que realizamos baseado em preço e prazo.

A partir disso, acredito que seja interessante adicionar uma maior cobertura de testes para o primeiro ponto e atualizar o teste com vcr para que realize uma nova chamada aos correios.

Como eu sei que essa parte de testes com Correios não é algo trivial e intuitivo eu tomei a liberdade de subir um diff da atualização que fiz caso queira seguir como exemplo. Acabei colocando usuário e senha do ambiente de teste público dos Corrreios sem mascarar. Atualmente no projeto é mascarado com XXX ou outros número, eu não vejo problema nisso em deixar público e facilitar futuras alterações, porém, só os mantenedores poderão opiniar melhor.

@memachado
Copy link

@nichmorgan De qualquer forma, sua sugestão era a correção que eu precisava no momento. Muito obrigado.

@erikhenrique Obrigado pela constribuição, dependendo da minha disponibilidade nos próximos dias tentarei seguir seu exemplo.

silviolleite
silviolleite previously approved these changes Aug 19, 2020
Copy link

@silviolleite silviolleite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@silviolleite silviolleite dismissed their stale review September 16, 2020 12:48

Aguardando fix relacionados aos testes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants